[Vanilla Bugfix] Fix Buggy Spotlight Behavior w/Event 35 - Enemy In Spotlight - #6

Development builds of 647b5f1:

• opents-Release-2bb25ec
• opents-Debug-2bb25ec

The links work without a GitHub account. Artifacts expire after 90 days, and this comment follows the latest successful build.

Changelog note, please

Right so, I am not well-versed in triggers, unfortunately, but something an AI review found.

The Follow restriction keys on object, which is NULL on most springing paths

taction.cpp:2067 returns false when object is NULL or not a building, and that return(false) also skips the fallback loop, so there's no degraded path — the action just does nothing.

That's fine for the detection path, where blight.cpp passes owner_building. But it isn't the only way a spotlight trigger springs:

• A multi-event trigger, e.g. [Enemy In Spotlight, Elapsed Time 10s]. The spotlight event latches on detection and the trigger actually springs on the TEVENT_TIME poll from logic.cpp:271, which calls Spring(TEVENT_TIME) with the default object = NULL.
• TAction_FORCE_TRIGGER, which calls Triggers[index]->Spring(NULL, CELL_NONE). "Force trigger to spring" on a spotlight trigger with a Follow action silently becomes a no-op.
• The house-tag TEVENT_ATTACKED poll at house.cpp:1779, also object = NULL.

In all three, Follow used to reach every tagged spotlight and now reaches none. Since IsDetectionSource is only ever true inside the two Spring calls in BuildingLightClass::AI, I think the fix wants to be "restrict to the source when there is one, otherwise behave as before" rather than an early return — probably as an extra predicate inside the existing loop (!restrict_to_source || ptr == object) instead of a parallel branch, which would also drop the duplicated six-condition validity check.

Plus needs docs - 52.md, 35.md, trigger-springing.md, and the chanelog note. Remember to rebase, I bumped the version.

There is also this AI found:

Consideration: could the Should_Spring fix be general?

Not blocking, but worth a decision either way. On a trigger with exactly one event the tripped bit can never do the job it exists for — there's no second event waiting to catch up — so all it can do is make all_sprung true on unrelated polls. That's the bug, and it applies to all 28 events satisfying Is_Time_Based() && Is_To_Flag_As_Tripped(), not just Event 35: Spied upon, Discovered by player, Selected, Pickup crate, Near waypoint, the damage-threshold events, the build events, and so on.

Gating the write rather than patching the read would cover all of them:

if (persistent && Class->FirstEvent->Next != NULL) {
    if (tevent->Is_Time_Based() && tevent->Is_To_Flag_As_Tripped()) {
        Flag_Event_Tripped(index);
    }
}

IsTripped has exactly one consumer (Is_Event_Tripped, called only from this loop; the other references are Compute_CRC and Serialize), and outcomes for the spotlight case come out identical to what you have — including suppressing the ENEMY_IN_SPOTLIGHT_REPEATING double-fire.

The counter-argument is real though: that changes behavior for 28 events at once, and some existing map may accidentally depend on the current latch. Staying narrow is defensible — I'd just want it to be an explicit decision, with a comment saying the latch is meaningless on a single-event trigger and that the clause is deliberately scoped to Event 35 for now. In that case tevent->Next == NULL && index == 0 reads off the loop variable already in hand; Class->FirstEvent->Next == NULL makes the reader first prove that tevent == Class->FirstEvent.

But I frankly again, don't understand triggers well enough to understand what in the world this implies.

Thanks for the review, Ziv. I went back through the spotlight-trigger path and adjusted the implementation based on your comments.

For TACTION_CHANGE_SPOTLIGHT_BEHAVIOR, I removed the earlier hard failure when object == NULL. Follow now restricts itself to the supplied building when a building object is available, but if the spring path does not provide an object, it falls back to the original tagged-spotlight loop instead of becoming a no-op.

That fixes the paths you pointed out such as elapsed-time completion and forced triggers, where the action may legitimately run with object == NULL.

I also kept the Should_Spring() fix intentionally narrow to Event 35 rather than generalizing the latch behavior for all latchable events. I agree that the generic write-side change is cleaner conceptually, but it would change behavior for a much wider set of trigger events, so I would prefer to keep this PR limited to the reproduced spotlight issue. I added a comment in the code making that decision explicit.

After making the changes, I went ahead and did another test on the spotlight behavior with two towers sharing the same tag.

The following cases are now working as intended:

• A single Event 35 trigger: the spotlight that detects the unit follows it, while the other spotlight continues sweeping.
• When the followed unit leaves range, the spotlight returns to Sweep without immediately re-detecting in the same frame.
• Two spotlights can independently detect and follow units.
• Attacking either spotlight after a single-event Event 35 detection no longer causes the repeated reset/spazzing behavior.

I also tested the multi-event case you mentioned:

• Event 35 - Enemy In Spotlight
• Event 13 - Elapsed Time (5s)

With added context that:

• S1 -> Spotlight Tower 1
• S2 -> Spotlight Tower 2
• U1 -> Unit 1
• U2 -> Unit 2

With a persistent/repeating trigger, S1 detects U1 and follows normally. After five seconds, the trigger fires, but Event 35 remains latched while the elapsed-time event is reset. The trigger then continues firing every five seconds.

Once S1 loses U1 and returns to Sweep, those repeated timer completions keep resetting spotlight behavior. S2 can still detect U1 or U2, but once this repeating state begins, only one spotlight appears able to actively detect/follow at a time while the other is affected by the repeated resets.

My current understanding is that the persistent trigger remembers only that Event 35 was satisfied, not which spotlight satisfied it. When the timer later completes with object == NULL, the action no longer has the original spotlight context available.

However, I also tested the same kind of multi-event setup using Event 54 (Enemy In Spotlight Repeating) instead of Event 35, and that behaves correctly. The spotlights and trigger continue to operate normally with multiple events.

That makes me wonder if these two spotlight events are intended to serve different roles:

• Event 35 appears to work naturally as an immediate/single-event spotlight trigger, where its latched state is useful only for the initial reaction.
• Event 54 appears better suited for multi-event spotlight conditions because it is re-tested rather than remaining latched in the same way.

If that distinction is intentional, I do not think adding persistent source tracking to Event 35 would be necessary. That would require storing which spotlight originally satisfied Event 35 across later trigger polls and would broaden this PR into trigger-state handling, potentially affecting serialization, CRC/network state, invalid-object handling, and shared-tag semantics.

Would you consider Event 35 effectively intended for the immediate/single-event case, with Event 54 being the appropriate spotlight event for multi-event triggers? If so, I would keep this PR narrow and document that distinction rather than adding persistent source tracking.

I’ll update 35.md, 52.md, trigger-springing.md, and the changelog once we confirm the intended distinction, and I’ll rebase against the latest version bump before finalizing the PR.